proxy: Verify either toplevel or target #2400

cgwalters · 2024-08-15T19:14:15Z

proxy: Move policycontext into global state

I am not aware of a reason not to just cache this for the life
of the proxy, like we do other global state.

Prep for further changes.

Signed-off-by: Colin Walters [email protected]

proxy: Verify either toplevel or target

An issue was raised in that a current Red Hat internal build system
was performing a signature just on the per-architecture manifest,
but the current proxy code is expecting a signature on the manifest
list.

To quote Miloslav from that bug:

Podman signs both the per-platform items and the top level,
and enforces signatures only on per-platform items. cosign,
by default, signs only the top level (and has an option to
sign everything), I’m not sure which one it enforces.
I don’t immediately recall other platforms.

We believe the current proxy code is secure since
we always require signatures (if configured) on the manifest
list, and the manifest list covers the individual manifests
via sha256 digest.

However, we want to support signatures only being present
on the per-arch manifest too in order to be flexible.

Yet, we can't hard switch to requiring signatures on the
per-arch manifests as that would immediately break anyone
relying on the current behavior of validating the toplevel.

Change the logic here to:

Verify signature on the manifest list, and cache the error (if any)
Fetch the manifest
Verify signature on the manifest
Allow if either signature was accepted; conversely, only error
if signature validation failed on both the manifest list and
manifest

This also switches things to cache the manifest upfront instead
of doing it lazily on GetManifest/GetConfig; in practice
callers were always immediately requesting those anyways.
The use case of just fetching blobs exists (e.g. to resume
an interrupted fetch), but is relatively obscure and
in general I think it's good to re-verify signatures on
each operation.

Signed-off-by: Colin Walters [email protected]

I am not aware of a reason not to just cache this for the life of the proxy, like we do other global state. Prep for further changes. Signed-off-by: Colin Walters <[email protected]>

cmd/skopeo/proxy.go

mtrmac

Thanks!

Implementation LGTM, just some naming nits.

cmd/skopeo/proxy.go

mtrmac

LGTM. Please mark “ready for review” when appropriate.

cmd/skopeo/proxy.go

cgwalters · 2024-08-15T21:08:25Z

OK I went to add tests and got burned by the fact that actually quay.io/coreos/11bot seemed to become a single-arch manifest list at some point...

But, using my own test image, I did at least verify that

git diff
diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go
index a4a90898..c3fdabed 100644
--- a/cmd/skopeo/proxy.go
+++ b/cmd/skopeo/proxy.go
@@ -294,9 +294,9 @@ func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (retReplyBu
                target = unparsedTopLevel
 
                // We're not using a manifest list, so require verification of the single arch manifest.
-               if toplevelVerificationErr != nil {
-                       return ret, toplevelVerificationErr
-               }
+               // if toplevelVerificationErr != nil {
+               //      return ret, toplevelVerificationErr
+               // }
        }
 
        img, err := image.FromUnparsedImage(ctx, h.sysctx, target)

(i.e. my original bug) will correctly cause the tests to fail.

It should be trivial to get someone with creds to upload a non-manifest-list fixture to quay.io/libpod - do you know who to ping? Chris?

(Only compile tested locally) An issue was raised in that a current Red Hat internal build system was performing a signature just on the per-architecture manifest, but the current proxy code is expecting a signature on the manifest list. To quote Miloslav from that bug: > Podman signs both the per-platform items and the top level, > and enforces signatures only on per-platform items. cosign, > by default, signs only the top level (and has an option to > sign everything), I’m not sure which one it enforces. > I don’t immediately recall other platforms. We believe the current proxy code is secure since we always require signatures (if configured) on the manifest list, and the manifest list covers the individual manifests via sha256 digest. However, we want to support signatures only being present on the per-arch manifest too in order to be flexible. Yet, we can't hard switch to requiring signatures on the per-arch manifests as that would immediately break anyone relying on the current behavior of validating the toplevel. Change the logic here to: - Verify signature on the manifest list, and cache the error (if any) - Fetch the manifest - Verify signature on the manifest - Allow if *either* signature was accepted; conversely, only error if signature validation failed on *both* the manifest list and manifest This also switches things to cache the manifest upfront instead of doing it lazily on `GetManifest/GetConfig`; in practice callers were always immediately requesting those anyways. The use case of just fetching blobs exists (e.g. to resume an interrupted fetch), but is relatively obscure and in general I think it's good to re-verify signatures on each operation. Signed-off-by: Colin Walters <[email protected]>

cgwalters · 2024-08-15T21:13:45Z

It'd also be useful to have sigstore-signed fixtures there of course.

mtrmac · 2024-08-15T21:15:26Z

@edsantiago probably has a good idea of what images exist / are appropriate to use from quay.io/libpod.

If we have no requirements for how the subject images {are, are not} signed: in the worst case, given a multi-platform image, we can read the index, choose an instance, and use a digest reference as a single-platform image.

mtrmac

(These are all non-blocking aesthetics.)

mtrmac · 2024-08-15T21:17:08Z

integration/proxy_test.go

+	config := proxyConfig{
+		policyFilePath: tempd + "/policy.json",
+	}
+	err := os.WriteFile(config.policyFilePath, []byte("{ \"default\": [ { \"type\":\"reject\"} ] }"), 0o644)


(This might be a tiny bit easier to read with raw string literals per https://go.dev/ref/spec#String_literals.)

mtrmac · 2024-08-15T21:18:51Z

integration/proxy_test.go

+	if err != nil {
+		err = fmt.Errorf("Testing optional image %s: %v", knownNotExtantImage, err)
+	}
+	assert.NoError(t, err)


assert.NoError(t, err, "testing image %s", knownNotExtantImage)

mtrmac · 2024-08-15T21:20:31Z

cmd/skopeo/proxy.go

@@ -701,9 +682,13 @@ func (h *proxyHandler) close() {
 		err := image.src.Close()
 		if err != nil {
 			// This shouldn't be fatal
-			logrus.Warnf("Failed to close image %s: %v", transports.ImageName(image.cachedimg.Reference()), err)
+			logrus.Warnf("Failed to close image %s: %v", transports.ImageName(image.img.Reference()), err)


(Pre-existing, non-blocking: using image.src.Reference() here would more obviously correct — but it ends up being the same value in the end.)

edsantiago · 2024-08-15T21:30:48Z

what images exist / are appropriate to use from quay.io/libpod.

Short answer: anything you want, it's under our control with multiple people having access.

Longer answer: CI VM images now include a local cache registry running on port 60333, with copies of all images required by buildah and podman. I'd like to extend that to skopeo too. Purpose is to reduce our exposure to quay flakes. Can signed images be served by a simple local registry process? If not, is there a way to make it possible?

mtrmac · 2024-08-15T21:36:34Z

sigstore signatures are ordinary objects on a registry, and can be served that way.

simple signing signatures are served using plain HTTP. It should not be very hard (set up a directory as a staging destination in registries.d, mirror the images, then publish contents of that directory as HTTP), but a bit of extra work.

mtrmac · 2024-08-15T21:38:43Z

… alternatively, we could also design a way to store simple-signing signatures on the registry (perhaps not using exactly the sigstore tag convention, but using OCI referrers as intended). We’ve been converging on sigstore as the future, so it’s unclear that it is necessary, and in some sense, having this convenient feature sigstore-only might motivate users to migrate, so maybe we should not be adding that.

cgwalters · 2024-08-15T22:15:35Z

Tangential to this PR landing but...per your comment on the bug:

(It would be easy enough to accept top-level-only signatures in c/image, but then users would lose the ability to create sparse mirrors with a subset of platforms; and it seems that there is a class of users which does care about that feature. Requiring pre-platform signatures, and preserving this feature for users, seems valuable.)

The next problem is, if we start trying to move bootc itself over to more like what podman/skopeo does (which includes hopefully unified storage) it will also necessarily imply that we'd need to ensure we replicate this "accept top-level-only" on at least the pull code path we use for bootc. I don't know what that would look like exactly...maybe some sort of new accept-toplevel-signatures flag in containers-registries.conf? Or a CLI flag, or...maybe we actually do just continue to use the proxy but expose a much higher level Copy API that can write to c/storage too.

(When we do that type of stuff though we'll definitely want a basic progress bar... #658 is related in that I think a lot of these "script skopeo on the CLI" cases would really more nicely generalize to "language-independent c/image RPC API")

mtrmac · 2024-08-15T22:22:31Z

Ugh, I’d prefer not to have that option, recognizing that it really might be unavoidable. That makes me more motivated to argue that this PR should just do what c/image does ~~in this PR~~, and we would see if anything breaks.

The implementation would probably be a new field in c/image/copy.Options; there is no obvious config file wired to that. (It would probably be containers.conf, except that currently Skopeo does not use it at all.)

cgwalters · 2024-08-16T12:11:07Z

If we have no requirements for how the subject images {are, are not} signed: in the worst case, given a multi-platform image, we can read the index, choose an instance, and use a digest reference as a single-platform image.

I looked at this, but it's a bit logistically annoying to do in the tests at the moment.

Short answer: anything you want, it's under our control with multiple people having access.

Ideally there'd be a git repository defining the images in quay.io/libpod - in this case a periodic sync like e.g. skopeo copy --multi-arch system docker://quay.io/centos/centos:stream9 docker://quay.io/libpod/centos-non-manifest-list or so. But, a one-time invocation of that is also fine.

cgwalters · 2024-08-16T16:52:23Z

Hmm. Maybe what would be helpful here is for the proxy to expose back to the client the signature state - especially the case when we detect only the manifest list was signed. I could then add something to bootc to loudly warn if this is the case...the problem is that there are going to be users that aren't paying attention to the journal/process-stdout for the often unattended systems that are a primary target for bootc. But it may help.

mtrmac · 2024-08-16T19:43:13Z

Maybe what would be helpful here is for the proxy to expose back to the client the signature state

Or have the proxy warn directly, and direct proxy’s stderr to a log?

cgwalters · 2024-08-21T18:34:27Z

Short answer: anything you want, it's under our control with multiple people having access.

@edsantiago ok but can you link me to the procedure or code or process for adding images there?

edsantiago · 2024-08-21T18:49:26Z

I've lost the thread here, so this is the question I am answering:

how does a given image X get pushed to quay.io/libpod ?
If that is not the question you intended, please ignore this, and clarify your question.

Answer: someone with credentials (me, @baude, not sure who else) pushes it.

If it is me, I'm neurotic about generating reproducible, documented, clearly tagged multiarch images. I will be glad to help craft something then push it. Please let me know how/where to begin with the image building process.

cgwalters · 2024-08-21T18:52:21Z

If it is me, I'm neurotic about generating reproducible, documented, clearly tagged multiarch images. I will be glad to help craft something then push it.

This sounds good to me but to achieve reproducible documented process I'd expect a a git repository to contain that, that's what I was looking for.

At this point I'm basically just repeating #2400 (comment)

edsantiago · 2024-08-21T19:01:29Z

IIUC you are referencing this part of the comment:

Ideally there'd be a git repository defining the images in quay.io/libpod - in this case a periodic sync like e.g. skopeo copy --multi-arch system docker://quay.io/centos/centos:stream9 docker://quay.io/libpod/centos-non-manifest-list or so. But, a one-time invocation of that is also fine.

That's not something I can ever imagine myself doing. I can envision myself doing a one-time skopeo copy ... /libpod/centos-non-manifest-list:20240821, preferably with a corresponding commit to the skopeo repo, in the section(s) that use this image, describing how/when/who that image was copied and what is so special about the source centos image.

You mentioned periodic. What would be the purpose of that?

cgwalters · 2024-08-21T20:17:41Z

I can envision myself doing a one-time skopeo copy ... /libpod/centos-non-manifest-list:20240821, preferably with a corresponding commit to the skopeo repo, in the section(s) that use this image, describing how/when/who that image was copied and what is so special about the source centos image.

Sure, that's fine. There's nothing special about the image, it could actually literally be any image; the goal is a fixture that is explicitly not a manifest list.

You mentioned periodic. What would be the purpose of that?

For the general reason that often creating something is only 15% of the work, the remaining 85% is maintaining it over time. Having images be pushed from a git repository with clear ownership and audibility is just better. But, if it's not true for the other images there today I wouldn't say we need to start doing it now just for this one.

That said, to combine these two things, I think it would actually be useful to have test fixtures set up with cosign; the current skopeo test suite does signing locally, which is fine, but it's handy to just have pre-made images with both good and bad signatures, etc. That'd be handy right now, but I can get by with just the not-manifest-listed fixture to start.

mtrmac · 2024-09-02T16:14:05Z

This sounds good to me but to achieve reproducible documented process I'd expect a a git repository to contain that, that's what I was looking for.

FWIW compare existing https://github.com/containers/skopeo/blob/main/systemtest/make-noarch-manifest . It’s not automated anywhere, but it is recorded in the same directory as the test that uses it.

github-actions · 2024-10-19T00:01:18Z

A friendly reminder that this PR had no activity for 30 days.

proxy: Move policycontext into global state

5e88bb0

I am not aware of a reason not to just cache this for the life of the proxy, like we do other global state. Prep for further changes. Signed-off-by: Colin Walters <[email protected]>

cgwalters force-pushed the proxy-validate-manifest-too branch from dd4b1c9 to 8de83ca Compare August 15, 2024 19:19

cgwalters commented Aug 15, 2024

View reviewed changes

cmd/skopeo/proxy.go Show resolved Hide resolved

mtrmac reviewed Aug 15, 2024

View reviewed changes

cmd/skopeo/proxy.go Outdated Show resolved Hide resolved

cmd/skopeo/proxy.go Outdated Show resolved Hide resolved

cmd/skopeo/proxy.go Outdated Show resolved Hide resolved

mtrmac reviewed Aug 15, 2024

View reviewed changes

cmd/skopeo/proxy.go Show resolved Hide resolved

cgwalters force-pushed the proxy-validate-manifest-too branch from 8de83ca to 732ddcc Compare August 15, 2024 20:42

mtrmac approved these changes Aug 15, 2024

View reviewed changes

mtrmac reviewed Aug 15, 2024

View reviewed changes

cmd/skopeo/proxy.go Outdated Show resolved Hide resolved

cgwalters force-pushed the proxy-validate-manifest-too branch from 732ddcc to 30a0e99 Compare August 15, 2024 21:07

cgwalters marked this pull request as ready for review August 15, 2024 21:07

cgwalters force-pushed the proxy-validate-manifest-too branch from 30a0e99 to 21bf7d8 Compare August 15, 2024 21:09

mtrmac reviewed Aug 15, 2024

View reviewed changes

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Sep 18, 2024

mtrmac mentioned this pull request Oct 14, 2024

Fix format string inconsistency causing a build failure #2440

Merged

github-actions bot added the stale-pr label Oct 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proxy: Verify either toplevel or target #2400

proxy: Verify either toplevel or target #2400

cgwalters commented Aug 15, 2024 •

edited

Loading

mtrmac left a comment

mtrmac left a comment

cgwalters commented Aug 15, 2024

cgwalters commented Aug 15, 2024

mtrmac commented Aug 15, 2024

mtrmac left a comment

mtrmac Aug 15, 2024

mtrmac Aug 15, 2024

mtrmac Aug 15, 2024

edsantiago commented Aug 15, 2024

mtrmac commented Aug 15, 2024

mtrmac commented Aug 15, 2024

cgwalters commented Aug 15, 2024

mtrmac commented Aug 15, 2024 •

edited

Loading

cgwalters commented Aug 16, 2024

cgwalters commented Aug 16, 2024

mtrmac commented Aug 16, 2024

cgwalters commented Aug 21, 2024

edsantiago commented Aug 21, 2024

cgwalters commented Aug 21, 2024

edsantiago commented Aug 21, 2024

cgwalters commented Aug 21, 2024

mtrmac commented Sep 2, 2024

github-actions bot commented Oct 19, 2024

proxy: Verify *either* toplevel or target #2400

Are you sure you want to change the base?

proxy: Verify *either* toplevel or target #2400

Conversation

cgwalters commented Aug 15, 2024 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

cgwalters commented Aug 15, 2024

cgwalters commented Aug 15, 2024

mtrmac commented Aug 15, 2024

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Aug 15, 2024

Choose a reason for hiding this comment

mtrmac Aug 15, 2024

Choose a reason for hiding this comment

mtrmac Aug 15, 2024

Choose a reason for hiding this comment

edsantiago commented Aug 15, 2024

mtrmac commented Aug 15, 2024

mtrmac commented Aug 15, 2024

cgwalters commented Aug 15, 2024

mtrmac commented Aug 15, 2024 • edited Loading

cgwalters commented Aug 16, 2024

cgwalters commented Aug 16, 2024

mtrmac commented Aug 16, 2024

cgwalters commented Aug 21, 2024

edsantiago commented Aug 21, 2024

cgwalters commented Aug 21, 2024

edsantiago commented Aug 21, 2024

cgwalters commented Aug 21, 2024

mtrmac commented Sep 2, 2024

github-actions bot commented Oct 19, 2024

proxy: Verify either toplevel or target #2400

proxy: Verify either toplevel or target #2400

cgwalters commented Aug 15, 2024 •

edited

Loading

mtrmac commented Aug 15, 2024 •

edited

Loading